Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix. toolbar disable,view comment not remove #3010

Merged
merged 16 commits into from
Sep 3, 2020
Merged

fix. toolbar disable,view comment not remove #3010

merged 16 commits into from
Sep 3, 2020

Conversation

nyufeng
Copy link
Contributor

@nyufeng nyufeng commented May 18, 2020 •
edited
Loading

  • fix Bug: Filters 'except' option not removing DebugToolbar comment for view #3002. toolbar disable,but view comment not remove.
  • delete filter argumets support. because, filter muset implements FilterInterface. before only receive request. so, if filter argumetns send to before, will get an error.
  • Alias resolution is no longer done in the run method, Filters::$filtersClass save alias to class
    Checklist:
  • FiltersTest. append aliases to config
  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@MGatner
Copy link
Member

MGatner commented Jun 2, 2020

Thank you @Instrye - this looks like quite a bit of work and some good changes. I don't have my head around it, I'll have to try another day. In the meantime, anybody else willing to give it a review?

@paulbalandan
Copy link
Member

This needs to be revised to incorporate changes in FilterInterface.

@nyufeng nyufeng closed this Jul 24, 2020
@nyufeng nyufeng reopened this Jul 24, 2020
@samsonasik
Copy link
Member

samsonasik commented Aug 12, 2020 •
edited
Loading

@MGatner could you re-check it? thanks

samsonasik
samsonasik reviewed Aug 12, 2020
View reviewed changes
system/View/View.php Outdated Show resolved Hide resolved
@MGatner @samsonasik
Use strict in_array check
b7d5e53 
Co-authored-by: Abdul Malik Ikhsan <[email protected]>
@MGatner
Copy link
Member

MGatner commented Aug 25, 2020

A few issues failing the static analysis check, but otherwise this looks good. Waiting on Travis. @samsonasik should we merge despite the CS failures? Or resolve those first?

@samsonasik
Copy link
Member

@MGatner I think code static analysis check need to incorporated first:

 ------ ----------------------------------------------------------------------- 
  Line   system/Filters/Filters.php                                             
 ------ ----------------------------------------------------------------------- 
  157    Access to an undefined property                                        
         CodeIgniter\Filters\Filters::$argumentsClass.                          
  183    Access to an undefined property                                        
         CodeIgniter\Filters\Filters::$argumentsClass.                          
  338    Access to an undefined property                                        
         CodeIgniter\Filters\Filters::$argumentsClass.                          
  495    Return typehint of method                                              
         CodeIgniter\Filters\Filters::processAliasesToClass() has invalid type  
         CodeIgniter\Filters\type.                                              
  497    Method CodeIgniter\Filters\Filters::processAliasesToClass() should     
         return CodeIgniter\Filters\type but return statement is missing.

MGatner
MGatner approved these changes Aug 26, 2020
View reviewed changes
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! @michalsn any thoughts before merge?

michalsn
michalsn requested changes Aug 26, 2020
View reviewed changes
system/Filters/Filters.php Show resolved Hide resolved
system/Filters/Filters.php Show resolved Hide resolved
system/Filters/Filters.php Outdated Show resolved Hide resolved
system/Filters/Filters.php Outdated Show resolved Hide resolved
@MGatner
Copy link
Member

MGatner commented Aug 30, 2020

@Instrye It looks like one more round of tweaks, if you're available for those.

@nyufeng
Copy link
Contributor Author

nyufeng commented Aug 31, 2020

😂I want to provide quality phpdoc, But my English level hinders me.

paulbalandan
paulbalandan reviewed Aug 31, 2020
View reviewed changes
system/Filters/Filters.php Outdated Show resolved Hide resolved
system/Filters/Filters.php Outdated Show resolved Hide resolved
system/Filters/Filters.php Outdated Show resolved Hide resolved
@nyufeng @paulbalandan
Update system/Filters/Filters.php
eaa1eb2 
update phpdoc

Co-authored-by: John Paul E. Balandan, CPA <[email protected]>
nyufeng and others added 2 commits September 1, 2020 15:42
@nyufeng @paulbalandan
[CI SKIP] Update system/Filters/Filters.php
4377bfc 
update phpdoc

Co-authored-by: John Paul E. Balandan, CPA <[email protected]>
@nyufeng
streamline the code
395d701
@nyufeng nyufeng requested a review from michalsn September 1, 2020 09:34
michalsn
michalsn approved these changes Sep 1, 2020
View reviewed changes
Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

You only have to resolve some conflicts with the develop branch and I think we're good to go.

@nyufeng
Copy link
Contributor Author

nyufeng commented Sep 3, 2020

@michalsn Everything's ready.

@michalsn michalsn merged commit 811d48d into codeigniter4:develop Sep 3, 2020
@michalsn
Copy link
Member

michalsn commented Sep 3, 2020

Thanks!

@nyufeng nyufeng deleted the toolbar-view-comment branch September 3, 2020 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulbalandan paulbalandan paulbalandan left review comments

@samsonasik samsonasik samsonasik left review comments

@MGatner MGatner MGatner approved these changes

@michalsn michalsn michalsn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: Filters 'except' option not removing DebugToolbar comment for view
5 participants
@nyufeng @MGatner @paulbalandan @samsonasik @michalsn